feat(code): Aligning review page for local and cloud#1829
Open
VojtechBartos wants to merge 2 commits intomainfrom
Open
feat(code): Aligning review page for local and cloud#1829VojtechBartos wants to merge 2 commits intomainfrom
VojtechBartos wants to merge 2 commits intomainfrom
Conversation
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/agent/src/server/schemas.test.ts
Line: 241-256
Comment:
**Prefer `it.each` over a `for` loop inside a single test**
A `for` loop inside one `it` block collapses all five assertions into a single test result, hiding which status value fails. Prefer a parameterised test:
```suggestion
it.each(["modified", "added", "deleted", "renamed", "untracked"])(
"accepts git/discard_file with status %s",
(status) => {
expect(
validateCommandParams("git/discard_file", {
filePath: "test.ts",
fileStatus: status,
}).success,
).toBe(true);
},
);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/code/src/renderer/features/code-review/hooks/useSandboxGit.ts
Line: 128-135
Comment:
**`method` cast loses type safety**
`sendSandboxCommand` accepts `method: string` and then passes it as `method as "git/changed_files"` to the mutate call. This silently bypasses the literal-union validation enforced by `sendCommandInput`, so a caller that passes a typo'd or unknown method name won't get a TypeScript error. Typing the parameter as `SandboxCommandMethod` (already exported from `schemas.ts`) would let the compiler catch mismatches at call sites.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(code): unified ReviewPage with Revi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+241
to
+256
| it("accepts git/discard_file with valid status", () => { | ||
| for (const status of [ | ||
| "modified", | ||
| "added", | ||
| "deleted", | ||
| "renamed", | ||
| "untracked", | ||
| ]) { | ||
| expect( | ||
| validateCommandParams("git/discard_file", { | ||
| filePath: "test.ts", | ||
| fileStatus: status, | ||
| }).success, | ||
| ).toBe(true); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
it.each over a for loop inside a single test
A for loop inside one it block collapses all five assertions into a single test result, hiding which status value fails. Prefer a parameterised test:
Suggested change
| it("accepts git/discard_file with valid status", () => { | |
| for (const status of [ | |
| "modified", | |
| "added", | |
| "deleted", | |
| "renamed", | |
| "untracked", | |
| ]) { | |
| expect( | |
| validateCommandParams("git/discard_file", { | |
| filePath: "test.ts", | |
| fileStatus: status, | |
| }).success, | |
| ).toBe(true); | |
| } | |
| }); | |
| it.each(["modified", "added", "deleted", "renamed", "untracked"])( | |
| "accepts git/discard_file with status %s", | |
| (status) => { | |
| expect( | |
| validateCommandParams("git/discard_file", { | |
| filePath: "test.ts", | |
| fileStatus: status, | |
| }).success, | |
| ).toBe(true); | |
| }, | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/server/schemas.test.ts
Line: 241-256
Comment:
**Prefer `it.each` over a `for` loop inside a single test**
A `for` loop inside one `it` block collapses all five assertions into a single test result, hiding which status value fails. Prefer a parameterised test:
```suggestion
it.each(["modified", "added", "deleted", "renamed", "untracked"])(
"accepts git/discard_file with status %s",
(status) => {
expect(
validateCommandParams("git/discard_file", {
filePath: "test.ts",
fileStatus: status,
}).success,
).toBe(true);
},
);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+128
to
+135
| export function useSandboxDiffUnstaged( | ||
| taskId: string | undefined, | ||
| options?: { enabled?: boolean; ignoreWhitespace?: boolean }, | ||
| ) { | ||
| const ctx = useCloudCommandContext(taskId); | ||
|
|
||
| return useQuery<string>({ | ||
| queryKey: [ |
Contributor
There was a problem hiding this comment.
method cast loses type safety
sendSandboxCommand accepts method: string and then passes it as method as "git/changed_files" to the mutate call. This silently bypasses the literal-union validation enforced by sendCommandInput, so a caller that passes a typo'd or unknown method name won't get a TypeScript error. Typing the parameter as SandboxCommandMethod (already exported from schemas.ts) would let the compiler catch mismatches at call sites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/code-review/hooks/useSandboxGit.ts
Line: 128-135
Comment:
**`method` cast loses type safety**
`sendSandboxCommand` accepts `method: string` and then passes it as `method as "git/changed_files"` to the mutate call. This silently bypasses the literal-union validation enforced by `sendCommandInput`, so a caller that passes a typo'd or unknown method name won't get a TypeScript error. Typing the parameter as `SandboxCommandMethod` (already exported from `schemas.ts`) would let the compiler catch mismatches at call sites.
How can I resolve this? If you propose a fix, please make it concise.
VojtechBartos
changed the title
VojtechBartos
force-pushed
the
vojtab/sandbox-git-commands
branch
from
3522138 to
4544d7b
Compare
Contributor
|
yo yo i think this looks good to me - fixing merge conflicts rn and i'll stack on top with some of my work! |
Member
Author
adboio
force-pushed
the
vojtab/sandbox-git-commands
branch
from
4544d7b to
7fb7b59
Compare
Contributor
|
@VojtechBartos today i'm working on bringing some of the PR actions we have for cloud over to local tasks and ideally those go out before beta release |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The idea
The desktop already talks to cloud sandboxes through
POST /commandon the agent server (proxied via the PostHog API).We extend this with
git/*andfs/*commands that query the sandbox's real git state — diffs, changed files, branches, file content — bypassing the agent session entirely. No new endpoints, no new auth. The sandbox commands return the same shapes as local tRPC git queries, so the desktop can use the same UI for both.Desktop ── POST /command { method: "git/diff_stats" } ──▶ PostHog API ──▶ Sandbox
Desktop ◀── { filesChanged: 2, linesAdded: 15, … } ─────────────────────┘
What changed
Agent server — 13 new commands (
git/changed_files,git/diff_cached,git/diff_unstaged,git/diff_stats,git/current_branch,git/file_at_head,git/stage_files,git/unstage_files,git/discard_file,git/sync_status,git/repo_info,git/diff_head,fs/read_file) with Zod validation, path traversal protection, and 28 tests.Desktop app — A
ReviewGitProvidercontext abstracts the transport. Components calluseReviewGit()and get diffs, stats, and file content without knowing whether the task is local or cloud.ReviewPageandCloudReviewPageare merged into a single component.DiffStatsBadgeworks for both execution types.PostHog backend — Serializer allowlist and param validation updated to proxy the new commands.